feat(macapp): route local VCS writes through the agent - #210

An independent review found that "only the StatusCommandRunning conformer
changes, so agent-routed and native writes classify identically by
construction" was false. CLIVCSWriter is genuinely untouched -- the
divergence was in what AgentCommandRunner FED it, which is harder to spot
than two classifiers, because nothing looks out of sync. Five defects, all
reproduced:

- A descendant holding the child's pipe (ssh ControlPersist, a daemonising
  credential helper, gpg-agent from commit -S) wedged run_exec's unbounded
  join forever. That thread is the one dispatch spawned, so its Permit never
  dropped, one of 32 ACTIVE slots -- shared with every read -- leaked, and
  is_busy() stayed true so the daemon never idle-exited. 32 occurrences kill
  the VCS service until wr-agent is killed by hand. drain_capped now publishes
  into a shared sink and run_exec detaches the readers after a 2s deadline,
  mirroring StatusCommandRunner's own bounded drain.wait and its reasoning.

- Every transport failure became CommandResult.launchFailed, whose doc
  reserves that value for "nothing ran". There is no cancel message in the
  protocol, so a cancelled or disconnected push completes host-side and was
  reported as never having launched -- inviting a retry that double-applies
  it. Split into neverRan (pre-send refusals, replies carrying an error) and
  outcomeUnknown, which carries the underlying error's own text; a new
  HostConnectionError.notDispatched keeps a local pool-full refusal on the
  honest side of that line. A closed connection deliberately still throws
  connectionLost, because connect()'s capabilities request routes through the
  same guard and LocalAgentVCS catches exactly that to spawn a stale-socket
  agent.

- Cancelling a gated write released <shared>/.jj/workroom-vcs.lock while
  wr-agent kept writing. JJSnapshotGate's "a started native operation retains
  this tail until its actual completion" held only by accident: a Process
  inside runBlocking cannot observe Swift cancellation, an agent round trip
  can. The gate now shields its operation once it holds the flock -- there,
  not in the command runner, because the thing holding the lock is the thing
  that must outlive the child, and shielding in the runner also covered
  remoteState's ungated reads and stranded connection slots on every
  superseded refresh.

- Non-network commands forwarded only PATH, so the child inherited
  GIT_AUTHOR_*, GIT_CONFIG_GLOBAL and HOME from whichever app launch first
  spawned the daemon -- silently authoring commits under a stale identity. No
  allowlist can close that; what git and jj read for identity, config, signing
  and hooks is open-ended. StatusCommandRunner.childEnvironment now builds one
  map for both paths and run_exec env_clear()s before applying it.

- A missing git/jj raised an Io error, so a missing tool was reported as a
  deleted workroom -- the exact conflation launchFailed's doc warns about, in
  the mirror direction. run_exec spawns /usr/bin/env <tool> exactly as native
  does, making 127/126 env's real exit codes.

Two follow-on rounds caught regressions in those fixes: env_clear() had
dropped the GIT_DIR/GIT_WORK_TREE family scrub (a commit requested in one
repository landed in another and reported success), and the timeout SIGKILL
fired after normal exits and could then be skipped entirely. Both closed. The
GIT_DIR scrub now lives in childEnvironment, so the NATIVE path gains it too
-- it never had one, and was the unsafe side of that pair.

Known gap, not closed here: an outcome-unknown result classifies as .other,
which VCSSyncPresentation.retryAction still offers a Retry for. The message is
truthful where it used to claim the command never ran, but suppressing the
button needs a new VCSRemoteFailure case -- a user-visible taxonomy change.

cargo test -p wr-agent 168 pass, clippy clean; make app-lint strict full-tree
clean; make app-test APP_TEST_FLAGS= 2753 pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The design doc claimed an untouched `CLIVCSWriter` made agent-routed and
native writes "classify identically by construction". A three-cycle review
falsified that, so the claim is replaced rather than softened: one classifier
fed divergent input reached opposite verdicts, and parity is now stated as the
three properties that actually produce it (same child environment, same
exit-code semantics for a missing tool, an error partition that separates
"never ran" from "outcome unknown"). Criterion 2 is recorded as met with one
named exception.

TODOS gains the standing "findings the /review pass didn't fix" entry the two
earlier VCS reviews already use: the retry affordance on an outcome-unknown
write, the unbounded `child.wait()` after SIGKILL, process-group vs
process-tree kill, the `writes: 8` pseudo-capability, the MAX_EXEC_STREAM /
MAX_RESPONSE mismatch, the 1 MiB request ceiling, read/write pool sharing, a
capabilities-negotiation disconnect missing the agent respawn, and seven test
gaps. The P1s are fixed in ed51faa; none of what remains is silent data loss.

Next Steps item 3 also records the two questions this milestone opened rather
than answered, both because the exec service is the thing Phase 3 moves
host-side: how a remote host resolves auth (every forwarded value is a path
into the CLIENT's filesystem), and that a transport carrying exec must
authenticate its peer to shell grade, not repository grade — there is no code
guard for that today.

macapp/AGENTS.md's working rules collect the failure that produced each rule,
so both instances go there: rule 1 gains the "X is unchanged, therefore
behaviour is unchanged" premise, whose fix is to verify everything reaching X;
rule 2 gains the three review-fix regressions (a dropped GIT_DIR scrub landing
a commit in the wrong repository, a cancellation shield stranding connection
slots, a SIGKILL guard that opened a never-kill path).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`AgentCommandRunner.outcomeUnknown` already told the truth — a cancelled or
disconnected write "may have completed" rather than "never ran" — but it
returned a SIGTERM-shaped `CommandResult`, which is indistinguishable from a
genuinely signaled `git`. So it classified as `.other`, and `.other`'s recovery
is a retry of the action that failed. An honest message with a button one click
from pushing twice.

The distinction had nowhere to live, so it gets a value:
`CommandResult.outcomeUnknown`, a sentinel exit code beside `launchFailed` and
for the same reason. That completes the partition every write classifier
depends on — `launchFailed` asserts nothing ran, a real exit code asserts a
known outcome, and this asserts neither. It is a state the native path cannot
produce: a `Process` either runs or fails to launch, and either way the answer
arrives.

`VCSRemoteFailure.outcomeUnknown` and `VCSCommitFailure.outcomeUnknown` carry it
to the UI, both classified before any output matching — the stderr is the
transport's own description, so matching git's prose against it could only
misfire.

`retryAction` answers `.fetch`, not nil. Nil would have left the failure tier
dead on a transient state; fetch is idempotent, the ahead/behind it returns is
exactly what resolves the unknown, and a successful one clears `lastFailure`.
Same shape as `.rejected → .pull`, where the recovery is deliberately not the
failed verb and the button is labelled with what it does.

Conditional on `lastAction`, which is the neighbouring property this broke
first: tier [13b] passes nil to ask what a READ failure needs on its own, and a
read that lost contact wrote nothing — answering `.fetch` there replaced its
"Try Again" re-read with a network action nobody asked for.

For a commit the doubt is usually resolved before the user sees it: `commit()`
compares the revision before and after, so a moved ref still reports
`.committedThenFailed`. The failure case is reached only when the repository
could not be re-read either, and its copy says so.

TODOS' standing entry for this is marked fixed, and the design doc's criterion 2
no longer records an exception.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

The sentinel and its partition survived review; six of the surfaces it reaches
did not. Rule 2 again — each finding is in the property next door, and none is
in the hunk that introduced it.

**The same defect, on the sibling surface.** `CommitSheet.isSpent` was true only
for `.landedThenFailed`, so `.failed(.outcomeUnknown)` left the Commit button
live under a notice telling the user to check the history first — one click from
the second commit, on the very taxonomy the last commit added a case to name.
`.outcomeUnknown` becomes its own `Phase`, for `.landedThenFailed`'s stated
reason: the button is the hazard, not the wording. Not folded INTO
`.landedThenFailed`, which asserts the commit was recorded.

**`.fetch` was generalised from one verb.** The recovery rule is IDEMPOTENCE,
not "never the failed verb". `.fetch` and `.abortRebase` are idempotent (a
second `git rebase --abort` exits "No rebase in progress"), so they are offered
back; only `.push`/`.pull` are swapped for a fetch. A lost abort was being
answered with Fetch, which resolves nothing about a parked rebase — and
`perform` allows an abort with no remote, so that click replaced the doubt with
an unrelated `.noRemote`.

**The disk is not output.** Returning the sentinel before the `.pull` +
`rebaseInProgress` check discarded the one piece of positive evidence available:
a lost pull left the same parked `rebase-merge` a killed one does, no read path
checks for it, and the offered Fetch then cleared `lastFailure` and restored an
ordinary toolbar over a workroom parked mid-rebase. It now asks the disk in the
same order the `timedOut` branch does, and accepts the same race for the same
reason. `commit()` already resolved its own unknown off the ref this way.

**Copy written for push, rendered for everything.** The remedy said "if the push
landed… push again", and tier [13b] routes READ failures through the same
`describe`/`remedy` pair — so a lost pull was told to push, and so was a failed
read. Now verb-neutral.

**Two surfaces stated the verdict the case exists to withhold.** The dialog
title and the spoken label both hardcoded "\(verb) failed" over a body saying it
may have succeeded; for a VoiceOver user that sentence IS the message. Both go
through one `headline`.

**An intent-to-add failure claimed a commit was sent.** `guard ita.ok else
{ return ita }` returned the staging result to be classified as a commit, and
skipped the rollback entirely — pre-existing, but the lost-contact case is the
one where those index entries may actually have landed, which is precisely the
`git stash` breakage that rollback exists to prevent. The rollback is now a
local function both exits use, and the staging failure is labelled as its own
step.

Also: `VCSCommitFailure.outcomeUnknown`'s doc and copy claimed the ref "could
not be re-read", which `commit()` does not enforce (a ref that read back
unchanged lands here too); `readRetryIsWorthwhile`'s comment called the new arm
unreachable when tier [13b] makes it the live path.

Tests: four new cases covering the idempotence rule, the parked rebase, the
heading, and the verb-neutral remedy — the gap that would have caught the rebase
finding was one missing `action: .pull` case. The existing sentinel-precedence
test matched a marker that never fired (lowercase); fixed.

Serial suite: 2759 passed, 1 skipped, 0 failures.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

Four of the writes review's defects — the unbounded join, the two caps that
can't both be satisfied, the stale child environment and the missing tool
reported as a deleted workroom — all lived in exactly this region. That is a
measurement, not a hunch, and the remaining TODOS items all edit code inside
it, so the coverage goes in first.

Rust (`vcs.rs`):

- The exec timeout clamp at both ends. 0 would kill a child before it started
  and an unbounded value would park an `ACTIVE` permit for as long as a caller
  asked. The ceiling's relation to `commitTimeout` moves to a `const _: () =
  assert!(…)`, so lowering it under 600s is a build error rather than a test
  someone runs later — a slow `pre-commit` hook must be cut short by its own
  limit, never by the transport.
- The exec envelope's `version != 1` guard, which nothing covered (the read
  path's is covered). It asserts `BackendVersion` specifically, because
  `RepositoryRouter` keys its fall-back-to-native decision on that case and any
  other error strands the user. The request asks for `git init spawned-anyway`
  and the test asserts that directory does not exist, so a guard that ever moves
  below the spawn fails here rather than passing on an error string.
- `drain_capped`'s cap AND its drain-past-cap property, the second through a
  REAL pipe. The cap bounds what is kept; refusing to read past it would block
  the child on a full pipe buffer forever — a hang, not a truncation. The test's
  writer thread only completes if the reader kept going.
- `latin1_bytes`' rejection branch, unreachable from the existing round trip
  (every byte 0x00-0xFF is representable), plus the boundary either side.

Swift (`AgentVCSIntegrationTests`):

- Both `writer(context:reader:)` guards, driven through `RepositoryRouter` —
  the only thing that can mint a `RepositoryContext`, and the real caller, so
  this also pins that the router does not swallow either error. The closed
  connection must report `connectionLost` and specifically NOT
  `VCSError.backendVersion`, the one error `writer(for:)` falls back to native
  writes on: a closed connection is not a pre-upgrade agent, and writing
  natively there would route around a connection the caller believes it is using.
- A capable agent actually ROUTING a write, asserted by reading the commit back
  out of the repository. Deliberately pins the DECISION rather than the `writes`
  count that currently encodes it, so replacing that count with an exec-service
  check (TODOS item 4) will not invalidate it.
- The write path's transport-failure mapping end to end through a real
  `VCSWriting` — the property `CommandResult.outcomeUnknown` exists for, which
  until now was only exercised against hand-built `CommandResult`s that cannot
  catch the writer losing the distinction on the way through.

cargo test -p wr-agent: 19 vcs tests pass, clippy and fmt clean.
AgentVCSIntegrationTests: 19 pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

…a kill

Two TODOS items, one change, because they share a root: `kill(-pid, …)` reaches
a process GROUP, and a descendant that calls `setsid` has left it.

**The tree.** Native has covered this since `ProcessTree` was written — its own
comment says "helpers spawned by git/gh can outlive the parent" — and the agent
had only the group kill. `descendants`/`kill_recorded` are the Rust half, via
`pgrep -P` for the reason that file records: `proc_listchildpids`' return value
is ambiguous across sources, and mis-reading it would aim a SIGKILL at the wrong
pid, which is worse than missing a child.

**The part native gets wrong too, and this does not.** The walk cannot happen at
kill time. A `setsid` descendant keeps its real ppid until its parent dies, so
`pgrep -P` finds it right up to that moment — and not after, because the orphan
re-parents to init and both handles are gone at once: the group it left, and a
lineage that no longer leads back to us. `killTree` is called at `timeout + 2`,
by which point a `git` that exited on SIGTERM has long been reaped, so it walks
an empty tree. Here the tree is SNAPSHOTTED when SIGTERM is sent and the
recorded pids are what gets SIGKILLed later. The group kill is kept alongside
rather than replaced: the tree misses a process that re-parented before the
snapshot, the group misses one that changed group, and the extra call is one
syscall.

The trade is a slightly wider PID-reuse window than native's, since the pids
were read earlier. Native accepted the same trade at `timeout + 2` for the same
reason — the alternative is leaving a process running against the user's
repository after the call that started it has returned.

**The wait.** `break 'wait child.wait()` was unbounded. The loop now polls to a
`REAP_GRACE` deadline after the SIGKILL and reports rather than blocking:
`timed_out` is already true on that path, so the client classifies it exactly as
it would a child we did reap — the outcome is the same fact, and the only
difference was whether this thread waited forever to state it. Deliberately not
`outcomeUnknown`: we know what happened to this command. The handle moves to a
detached thread, because Rust's `Child::drop` does not wait and dropping it
would leave a zombie nothing ever reaps.

Tests: a `setsid` grandchild that writes a marker after the whole escalation
window, asserted absent — it fails against the group-only kill, and it failed
against the first draft of this fix too, which walked the tree at kill time.
That is what turned up the snapshot requirement. Plus the walk's init floor and
termination.

The new test needs a real process table: under a sandbox that hides other
processes `pgrep` returns nothing and it fails indistinguishably from the bug.
Recorded in its doc comment, since it cost half an hour to diagnose once.

cargo test -p wr-agent: 174 pass, clippy and fmt clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

`writes: 8` described nothing on the side that answered it. The number counted
methods on `LocalVCSWriting`, a protocol that exists only in the app; wr-agent
implements one generic exec service and has never had eight write methods to
report. Nothing could keep the two in step, and the client compared them for
equality — so adding a ninth method to that Swift protocol, a change the exec
passthrough fully supports, would have made a completely capable agent look
incapable and silently dropped every user to native writes, with no log line.

The agent now reports `exec`, its exec service's own wire version, and the
client requires `>=` the version it sends. `>=` rather than `==` so an agent
that gains a version 2 does not refuse a client speaking 1: whether it still
accepts that request is the agent's call, and it answers it on the request
itself (`exec`'s `version != 1` guard), which is the only place with the
information to decide.

Nothing has shipped with `writes`, so it is removed rather than accepted
alongside — the whole stack is still draft.

The integration test written for this yesterday asserts the ROUTING decision
(a capable agent commits through the agent; an incapable one falls back) rather
than the number that encoded it, and passes unchanged across the swap. That was
the point of writing it that way.

cargo test -p wr-agent: 21 vcs tests pass. AgentVCS{Integration,Protocol}Tests:
31 pass. app-lint clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

`MAX_EXEC_STREAM` allows 4 MiB per stream and `MAX_RESPONSE` ceilings the reply
at 16 MiB, but a control byte JSON-escapes to six — so two full streams reach
48 MiB of wire form and `send` replaced the ENTIRE reply with `PartialData`.
That discards the exit status of a command that ran to completion: a known
outcome reported as an unknown one, which is the failure class the
`outcomeUnknown` partition was added to stop inventing. Codex reproduced it with
3 MiB of `0x01`.

Truncated in `exec` now, so `send`'s blanket replacement is never reached. That
replacement stays as-is for reads, where it is right — half a history is not a
history — and this is the one reply where a partial answer is strictly better
than none, because the exit code is the answer and the output is evidence.

By MEASURED escaped length, not by lowering the stream cap. Sizing
`MAX_EXEC_STREAM` for worst-case expansion would mean ~1.3 MiB per stream to
survive a case that essentially never occurs, permanently cutting ordinary
output to buy nothing; measuring costs one pass over the text and keeps
everything that fits.

stderr gets first call on the budget, capped at half, with stdout taking the
slack. It is what `CLIVCSWriter.classify` matches on, so losing it turns a
diagnosable failure into `.other` with git's own words missing — and on every
ordinary command it is tiny, so stdout gets nearly all of the budget anyway.

Head-truncated and silent, matching `drain_capped` and native's `readCapped`:
git and jj put the classifying line first, and injecting a "truncated" notice
into this text could only create a false positive against the markers that
classifier matches.

Tests: Codex's 3 MiB fixture, asserting the exit code survives and the
serialized reply lands under the ceiling, with a guard that fails if the fixture
ever stops exceeding the budget. Plus the accounting itself checked against
serde's own output, and truncation on a character boundary (a byte-wise cut
would panic on the slice).

cargo test -p wr-agent: 23 vcs tests pass, clippy and fmt clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

`AgentVCSConnection.connect` wrapped every negotiation failure as
`serviceUnavailable`, but `LocalAgentVCS` catches exactly `connectionLost` to
spawn wr-agent and retry. So a connection that dropped mid-`capabilities` was
routed around the stale-socket recovery and left the VCS service dead until the
app was restarted — over a stale socket, which is the common case rather than an
exotic one: the daemon leaves `session.sock` behind on any `pkill`.

`connectionLost` is now rethrown unchanged. Only that case: a `capabilities`
reply that arrives and reports an incompatible agent, and a handshake that times
out against a HUNG one, both stay `serviceUnavailable` — a respawn cannot fix
either, since the second candidate exits without binding while the first holds
the single-instance flock.

Same shape as the error-partition work earlier in this branch: a real
distinction collapsed into one value, and the caller downstream cannot act on
what it cannot see.

Test drives a socket that ACCEPTS and then hangs up, which is what a dead
agent's leftover socket does — a socket nothing listens on fails at connect and
never reached this wrapper, which is why nothing caught this before.

AgentVCSIntegrationTests: 20 pass. app-lint clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

`StatusCommandRunning.run(stdin:)` exists partly to sidestep `E2BIG`, and
routing it through a single un-chunked envelope put the ceiling straight back —
a lower one, since every NUL in `CLIVCSWriter`'s pathspec escapes to six bytes
on the wire. "Select all and commit" in a large repository worked natively and
failed through the agent, which is a plain regression against the path the agent
replaced.

Requests now chunk across envelopes on one stream, mirroring how replies always
have. Framed `[0x02][isFinal][bytes]` — a marker byte rather than a header on
every request, because a whole request is JSON and always starts `{`, so the two
are unambiguous and an UNCHUNKED request stays byte-for-byte what shipped
before. The common path pays nothing for this.

Gated on the agent's `exec >= 2` (bumped here): the framing is a wire change,
and a pre-chunking agent would read the marker as the start of a JSON document
and answer with a parse error. Below the ceiling the gate never applies.

Three neighbouring properties, each found by asking what this made worse rather
than whether it worked:

- **A refused request must poison its stream.** Chunks keep arriving after a
  rejection, and reassembling that tail as a fresh request would put a SECOND
  reply on a stream the client has already completed — which it treats as an
  unknown stream id, a protocol violation that tears down every OTHER in-flight
  request on the connection. Poisoned streams swallow their remainder silently
  and clear on the final chunk.
- **`is_busy()` must count partial requests.** A chunked request deliberately
  holds no `ACTIVE` permit until its last chunk lands, so a slow upload cannot
  occupy one of the 32 slots — but that left a client three chunks into five
  looking idle, and the daemon's idle-exit would drop the connection under it.
- **Per-stream caps alone are not a bound.** 32 streams × 16 MiB is 512 MiB of
  peer-controlled memory. A total cap (32 MiB) sits over the per-stream one.

Tests: a real 6000-file commit whose pathspec clears 1 MiB, driven through the
real client and the bundled agent, asserting the commit recorded every file —
a truncated payload would commit a prefix and still report ok. Plus reassembly,
interleaved streams, the poisoning, and a malformed header.

The fixture bug worth recording: the first draft used `.added` for the new
files, but a new file on disk reports `.untracked`, and only that routes through
intent-to-add. The commit then failed on pathspecs git had no index entry for —
which looks exactly like a truncated payload.

The `PARTIAL` tests serialize against each other on a test-local mutex: the map
is process-global, and they passed alone and failed in the parallel run. For the
same reason emptiness is asserted on the buffer rather than through `is_busy()`,
which ORs in the dispatch count that other tests are using.

cargo test -p wr-agent: 180 pass, clippy and fmt clean.
make app-test (serial): 2764 pass, 1 skipped. app-lint clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

Six findings, three of them one root cause, and that root cause was mine: the
chunking commit made `PARTIAL` a process-global keyed by stream id. Wrong three
ways at once.

**It could make the daemon immortal.** `is_busy()` consulted it and `serve`'s
idle check ORs `is_busy()`, so a single abandoned entry stopped `idle_since`
ever advancing. A client quitting after chunk 2 of 3 left `wr-agent serve`
resident until reboot — strictly worse than before the commit, where a vanished
client cost nothing. Its own doc claimed the leak was "bounded by the agent's
own lifetime", which is exactly what it unbounded.

**Stream ids are per connection.** The client resets `nextStream` to 1 on every
connect while this agent deliberately outlives the app, so one launch's
abandoned buffer could prepend itself to the next launch's identically-numbered
request — a parse error on the user's commit — or, if the stale entry was a
poison, swallow a live request whole and answer nothing, which the client waits
out and reports as `outcomeUnknown` for a command that provably never ran.

**The stream-count cap added an entry every time it refused one.** Poisoning is
an insert, so refusing the 33rd stream recorded a 33rd entry.

All three go away by making the map per-connection state, threaded through
`dispatch` exactly as `attached` and `token` already are. It dies with the
connection, ids cannot collide across connections, and `is_busy()` reverts —
`serve`'s own `connections > 0` already counts a live connection busy, so the
partial-request accounting was never needed. The stream-count cap now runs
BEFORE the byte caps, so a poison is only ever recorded for a stream there is
room to hold.

**A hung agent took the respawn path.** The `connectionLost` rethrow commit
claimed a handshake timing out against a HUNG agent stays `serviceUnavailable`;
`request`'s deadline resumed with `.connectionLost`, so it did not. New
`HostConnectionError.requestTimedOut` separates "the peer did not answer" from
"the connection died" — they want opposite recoveries, and a respawn cannot help
a wedged agent that still holds the flock. The greeting-read deadline stays
`connectionLost` on purpose: an agent that has not greeted within 2s is usually
one still starting up, which is what the retry loop is for.

**Two tests passed against their own reverted fix.**

- The dropped-handshake test's fixture accepted and closed immediately, which
  fails at the greeting read inside `connect`'s `runBlocking` closure — before
  the `catch` under test. It now greets with a valid `Hello` and then hangs up.
- The truncation test re-implemented the budget arithmetic instead of calling
  it. `bound_exec_streams` and `exec_timeout` are extracted so `exec` and the
  tests share one expression; deleting either from `exec` now reddens its test.
  The clamp test had the same defect and the same fix.

**Premise errors, which is what rule 1 is for.** `EXEC_SERVICE_VERSION` had been
inserted between `MAX_EXEC_TIMEOUT_MS`'s doc comment and its declaration, so a
paragraph about commit timeouts documented a version constant. `escaped_len` is
an upper BOUND — serde emits `\b` and `\f` as two bytes where it charges six —
but its test was named "agree" and sampled around exactly those two characters;
it now asserts equality where the two agree and never-under elsewhere.
`execVersion`'s doc claimed it "cannot drift" from `AgentExecRequest.version`;
they are separate declarations, so there is a test instead. The stale
`AgentCommandRunner.neverRan` caveat about oversized replies is gone — that is
the defect the truncation commit fixed.

Also recorded, not fixed: `>=` on the exec version is justified by a native
fallback that is not wired up. A `BackendVersion` error arriving in a REPLY
becomes `neverRan`/`launchFailed`, not the `VCSError.backendVersion` that
`RepositoryRouter` falls back on, so a future agent that DROPS version 1 would
fail every write as "launch failed". Unreachable today; noted where the `>=`
is, because that is where someone will need it.

cargo test -p wr-agent: 182 pass, clippy and fmt clean.
make app-test (serial): 2765 pass, 1 skipped. app-lint clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLpTyiW62gUJERhjwjcFFs

Four P3 entries from the #211 engineering review: rename
AgentVCSConnection once it carries files and events, extract the
shared service plumbing out of vcs.rs, share OS watchers across
subscriptions (accepted shortcut, upgrade at the Linux agent), and
raise or paginate the 4 MiB listing cap.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>